Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Split CRG functionality for U and S-modes #491

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

tariqkurd-repo
Copy link
Collaborator

@tariqkurd-repo tariqkurd-repo commented Dec 17, 2024

@jonwoodruff
Copy link
Contributor

To provide a bit of context, this was prompted by questions from @buxtonpaul about how to use the PTE bits, which involved setting up a null configuration for kernel-privileged pages. This then made obvious that the kernel pages were guarded by the same generation as user pages, and so sstatus would need to be swapped on kernel entry, which we believe is not the case currently. A better solution is to have a dedicated bit for the supervisor generation.

@jrtc27
Copy link
Collaborator

jrtc27 commented Dec 17, 2024

I misinterpreted the linked issue as being about kernel revocation and adding extra PTE bits, not about ensuring the kernel can actually load from kernel pages regardless of userspace's generation for userspace pages. Do we think that having sstatus.SCRG would form part of the story for kernel revocation? If there's a risk it's not what we'd want then it might be better to leave the sstatus bit (and PTE bit) reserved and say that there are no generation checks for S-mode pages. But probably it's something we'll need...

src/cheri-pte-ext.adoc Outdated Show resolved Hide resolved
src/cheri-pte-ext.adoc Outdated Show resolved Hide resolved
@nwf
Copy link
Collaborator

nwf commented Dec 17, 2024

@jrtc27 Has the right of it: kernel access to user memory should generally be gated, for revocation purposes, as if it were performed by the user program.

The existing implementation performs its page sweeps by bypassing the PTE bits and their load barrier effect by using the "direct map" region of the address space, manually translating virtual addresses to (offset) physical ones. These accesses would look, to hardware, like the kernel accessing kernel mappings.

There is a proposal for a privileged "as user capability load, bypassing the CRG fault" instruction that would let us simplify much of that logic down to allowing pages to be swept directly in virtual address space (and possibly even concurrently with address space manipulation, currently emphatically prohibited due to the direct access to physical memory involved), but that's still a lot of engineering and measurement to make sure it's a good idea to be done.

All of that to say, I'm not sure a "CRGS" bit is the right idea. Until someone cracks kernel sweeping revocation, a much more challenging problem than for userspace memory, the kernel's own access to virtual memory will be to pages that are almost surely parked in some capability-permissive but CRG-less PTE configuration, assuming we have one of those. If we don't, it's probably simpler to just not raise CRG faults on PTEs with their U bit clear?

@tariqkurd-repo tariqkurd-repo changed the title Split sstatus.CRG into CRGS/CRGU Split sstatus.CRG into SCRG/UCRG Dec 18, 2024
@jonwoodruff
Copy link
Contributor

jonwoodruff commented Dec 18, 2024

I'm certainly on-board with the far simpler, "non-user privileged pages don't ever get generation traps." With apologies to @tariqkurd-repo for dragging him through the mud, if we did agree on that one, would it be better to modify this pull request into that change to keep the history and discussion? Or start a new one? (Such a modification would basically be reverting most/all of it and adding a note somewhere. I guess.)

@tariqkurd-repo tariqkurd-repo changed the title Split sstatus.CRG into SCRG/UCRG SPlit CRG funcitonality for U and S-modes Jan 9, 2025
@tariqkurd-repo tariqkurd-repo changed the title SPlit CRG funcitonality for U and S-modes Split CRG functionality for U and S-modes Jan 10, 2025
@tariqkurd-repo
Copy link
Collaborator Author

tariqkurd-repo commented Jan 10, 2025

Two big changes here in the last commit:

  1. remove sstatus.SCRG, and leave kernel space revocation for future
  2. make PTE.CW mandatory, so the default is to not allow tagged caps to be stored for legacy OSs

and so Zcheripte includes CRG but not CW

Overall, this PR:

  1. renames sstatus.CRG to sstatus.UCRG and makes it only tested in user mode
  2. has no support for kernel space revocation (sstatus.SCRG is not specified)
  3. makes PTE.CW mandatory and PTE.CRG+status.UCRG optional but strongly recommended

@tariqkurd-repo tariqkurd-repo requested a review from jrtc27 January 10, 2025 11:20
src/cheri-pte-ext.adoc Outdated Show resolved Hide resolved
src/cheri-pte-ext.adoc Outdated Show resolved Hide resolved
|===

^1^ This is the current privilege mode, not the effective mode of the access and so is not affected by <<sstatusreg_pte,sstatus>>.SUM.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think sstatus.SUM has anything to do with effective privilege mode, which regards mstatus.prv and mstatus.mpp. So I think this needs to be rephrased.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If mstatus.MPRV=1 then all data memory accesses are performed as if in mode mstatus.MPP. So if mstatus.MPP=01 (S) then sstatus.SUM applies to all M-mode's data memory accesses.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And it must be affected by SUM. It is vital that, when the kernel copies memory to/from userspace, it follows exactly the same restrictions as userspace. Otherwise you could use a system call as a gadget to copy a to-be-revoked capability from a not-yet-swept page to a swept one.

Copy link
Collaborator

@jrtc27 jrtc27 Jan 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is, whether UCRG is used or not must be a function solely of the U bit, not the current mode as well. (Either the access is being done from U-mode, in which case that's always going to be set, or it's not, in which case the effective mode must be S-mode, whether in S-mode or in M-mode via mstatus.MPRV, and for it not to be faulting unconditionally already in the base privileged spec sstatus.SUM must also be set)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you are right about the SUM bit for sure, so the " not affected by <<sstatusreg_pte,sstatus>>.SUM" is wrong. I think this is the first time we have page bits that only apply in user mode I think?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For RISC-V I believe so, yes

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok - so it should be the effective mode of the memory access instead - should be a simple text replacement.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The effective mode is still S-mode with SUM. SUM just determines whether or not S-mode accessing pages with the U bit faults. That's why I said:

That is, whether UCRG is used or not must be a function solely of the U bit, not the current mode as well.

No mention of "effective mode". If the U bit is set, CRG exists and UCRG is checked against. If the U bit is clear, there is no CRG bit. There does not need to be any mention of the mode, because that is both necessary and sufficient.

| 1 |= <<sstatusreg_pte,sstatus>>.CRG | Normal operation
|PTE.CW |Mode^1^ |PTE.CRG |Load/AMO
| 0 | S/U | X | Clear loaded tag
| 1 | U |&#8800; <<sstatusreg_pte,sstatus>>.UCRG | Page fault, or page fault if tag is set^1^
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
| 1 | U |&#8800; <<sstatusreg_pte,sstatus>>.UCRG | Page fault, or page fault if tag is set^1^
| 1 | U |&#8800; <<sstatusreg_pte,sstatus>>.UCRG | Page fault, or page fault if tag is set

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to merge these footnotes so that the ^1^ here refers to the ^1^ further down.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

there's no mechanism for separating kernel and usespace revocation sweeps
7 participants